Skip to content

Preserve rollup invalidations during concurrent refreshes - #1691

Open
skyfallwastaken wants to merge 1 commit into
fix-partial-import-rollupsfrom
fix-rollup-invalidation-races
Open

Preserve rollup invalidations during concurrent refreshes#1691
skyfallwastaken wants to merge 1 commit into
fix-partial-import-rollupsfrom
fix-rollup-invalidation-races

Conversation

@skyfallwastaken

Copy link
Copy Markdown
Member

Summary of the problem

A refresh could clear a newer invalidation and publish inconsistent aggregates after a historical correction.

Describe your changes

Track a durable per-user generation and build rollups in a repeatable-read transaction. Keep follow-up jobs enqueueable while a refresh runs and queue another refresh if its snapshot is already stale. Includes an additive database migration; apply it before running the new code.

Screenshots / Media

Not applicable: backend-only changes.

@skyfallwastaken
skyfallwastaken force-pushed the fix-rollup-invalidation-races branch from 59c42e6 to 08cd15f Compare September 6, 2026 13:16
@greptile-apps

greptile-apps Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR replaces cache-backed dashboard invalidation with a durable per-user generation, builds rollups from a repeatable-read snapshot and permits a pending per-user follow-up refresh.

  • Adds users.dashboard_rollup_generation and stores the consumed generation in the total rollup.
  • Reworks refresh scheduling and GoodJob concurrency so invalidations during a refresh can trigger another job.
  • Adds database-backed concurrency coverage and updates architecture documentation.
  • The read-triggered scheduling change can still allow one dashboard response to combine fresh aggregate calculations with stale rollup fragments.

Confidence Score: 4/5

The PR should not merge until fingerprint-detected staleness also prevents stale dashboard fragments from being returned.

The durable generation and repeatable-read refresh address the primary concurrent invalidation race, but replacing schedule_for with enqueue_for leaves the generation clean when the existing source fingerprint detects staleness, allowing one dashboard response to mix fresh and stale data.

Files Needing Attention: app/services/dashboard_stats.rb

Important Files Changed

Filename Overview
app/jobs/dashboard_rollup_refresh_job.rb Separates durable invalidation from enqueueing, serializes active refreshes and schedules a follow-up when generations differ.
app/models/dashboard_rollup.rb Replaces the disposable dirty cache marker with a persisted generation comparison.
app/services/dashboard_rollup_refresh_service.rb Builds and replaces the complete rollup set inside a repeatable-read transaction while recording its source generation.
app/services/dashboard_stats.rb Read-triggered repairs now enqueue without making all stale rollup fragments fail their validity checks.
test/services/dashboard_rollup_concurrency_test.rb Exercises a correction during refresh, coherent snapshots, follow-up scheduling and invalidation durability across cache loss.
db/migrate/20260906124526_add_dashboard_rollup_generation_to_users.rb Adds the durable per-user rollup generation with a non-null zero default.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  H[Heartbeat or historical correction] --> G[Increment durable user generation]
  G --> Q[Enqueue refresh]
  Q --> T[Repeatable-read transaction]
  T --> S[Read heartbeat snapshot and generation]
  S --> R[Atomically replace rollup rows]
  R --> C{Stored generation matches current generation?}
  C -->|Yes| D[Rollup is clean]
  C -->|No| F[Enqueue follow-up refresh]
  V[Dashboard detects fingerprint mismatch] --> E[Enqueue without generation increment]
  E --> X[Other fragments may still accept stale rollups]
Loading
Prompt To Fix All With AI
### Issue 1
app/services/dashboard_stats.rb:269
**Stale fragments remain trusted**

When the total row’s heartbeat fingerprint is stale but its generation still matches, this only enqueues a refresh without marking the rollup dirty. The default dashboard then accepts the old activity graph, today stats, filter options and coding rhythm, causing the response to mix fresh aggregate calculations with stale fragments until the asynchronous refresh completes.

```suggestion
    DashboardRollupRefreshJob.schedule_for(user.id, wait: wait)
```

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "Preserve rollup invalidations across con..." | Re-trigger Greptile

def schedule_rollup_refresh(wait:)
return if @rollup_refresh_scheduled
DashboardRollupRefreshJob.schedule_for(user.id, wait: wait)
DashboardRollupRefreshJob.enqueue_for(user.id, wait: wait)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Stale fragments remain trusted

When the total row’s heartbeat fingerprint is stale but its generation still matches, this only enqueues a refresh without marking the rollup dirty. The default dashboard then accepts the old activity graph, today stats, filter options and coding rhythm, causing the response to mix fresh aggregate calculations with stale fragments until the asynchronous refresh completes.

Suggested change
DashboardRollupRefreshJob.enqueue_for(user.id, wait: wait)
DashboardRollupRefreshJob.schedule_for(user.id, wait: wait)

Knowledge Base Used:

Prompt To Fix With AI
This is a comment left during a code review.
Path: app/services/dashboard_stats.rb
Line: 269

Comment:
**Stale fragments remain trusted**

When the total row’s heartbeat fingerprint is stale but its generation still matches, this only enqueues a refresh without marking the rollup dirty. The default dashboard then accepts the old activity graph, today stats, filter options and coding rhythm, causing the response to mix fresh aggregate calculations with stale fragments until the asynchronous refresh completes.

```suggestion
    DashboardRollupRefreshJob.schedule_for(user.id, wait: wait)
```

**Knowledge Base Used:**
- [Dashboard rollups and caching](https://app.greptile.com/mahadk/-/custom-context/knowledge-base/hackclub/hackatime/-/docs/dashboard-rollups-and-caching.md)
- [Dashboards and user insights](https://app.greptile.com/mahadk/-/custom-context/knowledge-base/hackclub/hackatime/-/docs/dashboards-and-user-insights.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant